Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the project’s Python dependencies to newer major versions (notably Pydantic v2 and ops v3) and adapts the OpenSearch charm library models to the new Pydantic APIs.
Changes:
- Bump dependency constraints for
pydantic,ops,ops-scenario, anddata-platform-helpers, and refreshpoetry.lock. - Migrate
lib/charms/opensearch/v0/models.pyfrom Pydantic v1 validators/root models to Pydantic v2 equivalents. - Update the OpenSearch security test resource (
internal_users.yml) hashes/order.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
tests/unit/resources/config/opensearch-security/internal_users.yml |
Updates internal demo user entries (ordering + bcrypt hashes) used by unit test resources. |
pyproject.toml |
Bumps core and group dependency constraints to Pydantic v2 / ops v3 and updates related tooling deps. |
poetry.lock |
Lockfile refresh reflecting new resolved dependency graph (Pydantic v2, ops v3, new transitive deps). |
lib/charms/opensearch/v0/models.py |
Refactors model/validator definitions to Pydantic v2 (model_validator, RootModel, typing updates). |
Comments suppressed due to low confidence (3)
lib/charms/opensearch/v0/models.py:563
GcsRelData.validate_core_fieldsis registered withmodel_validator(mode="before")but accessescreds.secret_key. Inmode="before",credentialsis still raw input (commonly a dict), so this can fail with attribute errors. Prefermode="after"and validate againstself.credentials, or handle both dict and model cases.
@model_validator(mode="before")
@classmethod
def validate_core_fields(cls, values): # noqa: N805
"""Validate the core fields of the gcs relation data."""
creds = values.get("credentials")
if not creds or not creds.secret_key:
raise ValueError("Missing fields: secret-key")
if not values.get("bucket"):
raise ValueError("Missing field: bucket")
lib/charms/opensearch/v0/models.py:338
S3RelData.validate_core_fieldsruns asmodel_validator(mode="before"), but it assumesvalues["credentials"]is already aS3RelDataCredentialsinstance (s3_creds.access_key/secret_key). Inmode="before", nested models haven’t been parsed yet, socredentialswill often be a plain dict and this will raise at runtime. Usemode="after"(accessself.credentials) or update the logic to handle raw dicts.
@model_validator(mode="before")
@classmethod
def validate_core_fields(cls, values): # noqa: N805
"""Validate the core fields of the S3 relation data."""
if (
not (s3_creds := values.get("credentials"))
or not s3_creds.access_key
or not s3_creds.secret_key
):
raise ValueError("Missing fields: access_key, secret_key")
lib/charms/opensearch/v0/models.py:463
AzureRelData.validate_core_fieldsis amodel_validator(mode="before")but it treatsvalues["credentials"]as anAzureRelDataCredentialsobject (creds.storage_account/secret_key). Inmode="before",credentialsmay be a dict, causing attribute errors during validation. Switching this validator tomode="after"(and usingself.credentials) avoids this class of failures.
@model_validator(mode="before")
@classmethod
def validate_core_fields(cls, values): # noqa: N805
"""Validate the core fields of the azure relation data."""
if (
not (creds := values.get("credentials"))
or not creds.storage_account
or not creds.secret_key
):
raise ValueError("Missing fields: storage_account, secret_key")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lib/charms/opensearch/v0/models.py
Outdated
|
|
There was a problem hiding this comment.
Trailing whitespace on this blank line should be removed to avoid lint/style issues.
pyproject.toml
Outdated
| ops = "^2.22.0" | ||
| ops = "^3.1.0" | ||
| # data_platform_libs/v0/upgrade.py | ||
| # grafana_agent/v0/cos_agent.py requires pydantic <2 |
There was a problem hiding this comment.
This comment says grafana_agent/v0/cos_agent.py requires pydantic <2, but this PR bumps pydantic to >=2.0 and the referenced library already contains a Pydantic v2 branch. Please update/remove the comment so it reflects the current dependency constraints.
| # grafana_agent/v0/cos_agent.py requires pydantic <2 | |
| # grafana_agent/v0/cos_agent.py uses the Pydantic v2-compatible branch (requires pydantic >=2.0) |
| pydantic = ">= 2.0" | ||
| cryptography = "^45.0.4" | ||
| jsonschema = "^4.24.0" | ||
| data-platform-helpers = "^0.1.4" | ||
| data-platform-helpers = ">=0.1.7" |
There was a problem hiding this comment.
The dependency constraints pydantic = ">= 2.0" and data-platform-helpers = ">=0.1.7" have no upper bounds, which can allow future major releases (e.g. Pydantic v3) to be selected and break the project unexpectedly. Consider bounding these (e.g. <3 for pydantic, and an appropriate < cap for data-platform-helpers) to keep upgrades intentional.
lib/charms/opensearch/v0/models.py
Outdated
| @classmethod | ||
| def set_props(cls, values): # noqa: N805 | ||
| """Generate the attributes depending on the input.""" | ||
| if None not in list(values.values()): |
There was a problem hiding this comment.
App.set_props is a model_validator(mode="before"), so values is the raw input dict (missing keys aren’t present). The early return if None not in list(values.values()) will incorrectly skip deriving name/model_uuid/short_id when callers provide only id (common in this codebase), leaving those fields as None. Consider switching this to mode="after" and populating derived fields on the model instance, or explicitly normalizing missing keys to None before checking.
| if None not in list(values.values()): | |
| # If all key properties are already populated, skip derivation. | |
| if all(values.get(k) is not None for k in ("id", "name", "model_uuid", "short_id")): |
This PR is bumping pydantic to use v2 and bump ops to latest version. pydantic v2 introduce a set of new features and the migration was based on the migration guide.